Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename to Valkey for lolwut command #1559

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

sarthakaggarwal97
Copy link
Contributor

While trying out few commands in the codebase, I found that upon executing lolwut command on unstable, it still returns Redis Ver. Thought, since we are on unstable, maybe we should remove this?

Let me know if that isn't the case.

Screenshot 2025-01-14 at 11 48 59 AM

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.00%. Comparing base (12ec3d5) to head (cf8971d).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1559      +/-   ##
============================================
+ Coverage     70.98%   71.00%   +0.01%     
============================================
  Files           121      121              
  Lines         65176    65176              
============================================
+ Hits          46264    46276      +12     
+ Misses        18912    18900      -12     
Files with missing lines Coverage Δ
src/lolwut.c 21.42% <100.00%> (+21.42%) ⬆️
src/script.c 87.69% <100.00%> (ø)

... and 10 files with indirect coverage changes

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, but we can avoid hard-coding the name.

(It's weird that we have SERVER_TITLE but we decided to name the version variable VALKEY_VERSION instead of SERVER_VERSION. Maybe there was a reason for that. I don't remember the discussion.)

src/lolwut.c Outdated Show resolved Hide resolved
@ranshid
Copy link
Member

ranshid commented Jan 15, 2025

Trying to understand the issue. AFAIK changing the lolwut can break some clients using it in order to get the current verison.
I agree that providing the unstable version is problematic and we might want to do the same as we did with the info server version (i.e use fixed 7.2.4).

The other issue is that we also have the "Easter eggs" lolwat (which are somewhat broken, for example you cannot enter lolwut which are also presenting the VALKEY_VERSION version and Redis brand (I wonder if these we can be changed).

In anycase I think this should be marked as a breaking change

@ranshid ranshid added breaking-change Indicates a possible backwards incompatible change client-changes-needed Client changes are required for this feature rebranding Valkey is not Redis labels Jan 15, 2025
@ranshid
Copy link
Member

ranshid commented Jan 27, 2025

@sarthakaggarwal97 I just looked. seems like @zuiderkwast suggestion to control this feature via extended-redis-compat might be fine.

@sarthakaggarwal97
Copy link
Contributor Author

@ranshid @zuiderkwast if I understand this correctly, extended-redis-compat is stemming from #274 right?

I see we have a configuration available in valkey.conf as extended-redis-compatibility. So we can continue with this change relying on this configuration?

@zuiderkwast
Copy link
Contributor

@ranshid @zuiderkwast if I understand this correctly, extended-redis-compat is stemming from #274 right?

I see we have a configuration available in valkey.conf as extended-redis-compatibility. So we can continue with this change relying on this configuration?

That's right. I think it's OK to use this config for lolwut. We should make a TSC decision about it though.

I remember when we added this config, there were some concerns that it will give us a lot of extra maintenance work, so we decided to limit the scope to only a few things and to make it temporary and be phased out in 9.0. See the test "Extended Redis Compatibility config" in tests/unit/other.tcl

@hwware hwware added the major-decision-pending Major decision pending by TSC team label Jan 30, 2025
src/lolwut.c Outdated Show resolved Hide resolved
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
@ranshid
Copy link
Member

ranshid commented Feb 3, 2025

Will just ask again:
why not rebrand the lolwut5Command and lolwut6Command?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates a possible backwards incompatible change client-changes-needed Client changes are required for this feature major-decision-pending Major decision pending by TSC team rebranding Valkey is not Redis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants